-
Notifications
You must be signed in to change notification settings - Fork 40
Add support for administrative operations in Blob adapter #3104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds administrative operation support for the Azure Blob storage adapter. The changes are extensive, including new admin implementations, configurations, utility classes, and a suite of integration and unit tests.
The overall implementation looks solid and follows existing patterns in the codebase. I've found a few areas for improvement:
- A potential bug in the test utilities where teardown logic might fail if certain resources don't exist.
- A violation of the immutability contract in one of the new metadata classes.
- A minor code style issue with redundant formatting.
Details are in the specific comments. Otherwise, the changes are well-structured and the test coverage is good.
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageTableMetadata.java
Show resolved
Hide resolved
...c/integration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageAdminTestUtils.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blob/BlobWrapper.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for administrative operations for the Azure Blob storage adapter. The changes are well-structured, with clear separation of concerns between the generic object storage logic and the specific Blob storage implementation. The use of a wrapper interface for the storage client is a good design choice. I've included a couple of comments for potential improvements, one regarding the robustness of a delete operation and another about handling concurrent administrative operations.
core/src/main/java/com/scalar/db/storage/objectstorage/blob/BlobWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces administrative operation support for the Azure Blob storage adapter. The changes include a new ObjectStorageAdmin implementation that manages table and namespace metadata as objects within the storage. It also adds a generic ObjectStorageWrapper interface to abstract away the specifics of different object storage services, with an initial implementation for Azure Blob. The changes are well-structured and include comprehensive unit and integration tests. I have a couple of suggestions to reduce code duplication in the ObjectStorageAdmin class for better maintainability.
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces Azure Blob Storage support for ScalarDB by implementing a new object storage adapter layer. The implementation provides admin functionality for metadata management without supporting full distributed storage operations.
Key changes:
- Adds Azure Blob Storage wrapper and configuration classes with comprehensive storage operations
- Implements ObjectStorageAdmin for managing table and namespace metadata stored in JSON format
- Adds extensive test coverage including unit tests and integration tests with Azurite emulator
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| core/build.gradle | Adds Azure Blob SDK dependency and new integration test source set |
| build.gradle | Adds Azure Blob Storage version configuration |
| .github/workflows/ci.yaml | Adds CI workflow for Azure Blob integration tests using Azurite emulator |
| BlobConfig.java | Configuration class parsing blob storage connection details and parallel upload settings |
| BlobWrapper.java | Azure Blob Storage implementation with CRUD operations and ETags for versioning |
| BlobProvider.java | Service provider interface implementation for blob storage |
| ObjectStorageAdmin.java | Admin operations managing metadata as JSON objects in blob storage |
| ObjectStorageUtils.java | Utility methods for constructing object keys and configuration |
| Serializer.java | JSON serialization/deserialization using Jackson |
| ConcatenationVisitor.java | Visitor pattern implementation for building concatenated partition keys |
| Multiple test files | Comprehensive unit and integration test coverage |
| ConsensusCommitSpecificIntegrationTestBase.java | Makes several test methods public for better testability |
| CoreError.java | Adds error codes for unsupported object storage operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/storage/objectstorage/blob/BlobProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageWrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageUtils.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageTableMetadata.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfigTest.java
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfigTest.java
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfigTest.java
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfigTest.java
Show resolved
Hide resolved
...va/com/scalar/db/transaction/consensuscommit/ConsensusCommitSpecificIntegrationTestBase.java
Show resolved
Hide resolved
...rc/integration-test/java/com/scalar/db/storage/objectstorage/BlobWrapperIntegrationTest.java
Outdated
Show resolved
Hide resolved
komamitsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left several comments. PTAL!
...n-test/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapperIntegrationTestBase.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfig.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/scalar/db/storage/objectstorage/BlobStorageConfigTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageWrapper.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java
Outdated
Show resolved
Hide resolved
josh-wong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments and suggestions. PTAL!
...om/scalar/db/storage/objectstorage/ConsensusCommitAdminIntegrationTestWithObjectStorage.java
Outdated
Show resolved
Hide resolved
...om/scalar/db/storage/objectstorage/ConsensusCommitAdminIntegrationTestWithObjectStorage.java
Outdated
Show resolved
Hide resolved
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
josh-wong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some minor suggestions. Other than that, LGTM. Thank you!🙇🏻♂️
...om/scalar/db/storage/objectstorage/ConsensusCommitAdminIntegrationTestWithObjectStorage.java
Outdated
Show resolved
Hide resolved
...va/com/scalar/db/storage/objectstorage/ObjectStorageAdminCaseSensitivityIntegrationTest.java
Outdated
Show resolved
Hide resolved
...gration-test/java/com/scalar/db/storage/objectstorage/ObjectStorageAdminIntegrationTest.java
Outdated
Show resolved
Hide resolved
...orage/objectstorage/SingleCrudOperationTransactionAdminIntegrationTestWithObjectStorage.java
Outdated
Show resolved
Hide resolved
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Co-authored-by: Toshihiro Suzuki <[email protected]>
Description
This PR adds the Admin implementation for Blob adapter, enabling administrative operations over Azure Blob storage.
Support for S3 and GCS, and Storage operations will be addressed in subsequent PRs.
Related issues and/or PRs
N/A
Changes made
DistributedStorageAdminimplementation for Object Storage.ObjectStorageWrapperimplementation for Azure Blob storage.Checklist
Additional notes (optional)
N/A
Release notes
Added support for administrative operations over Azure Blob Storage.